Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mpi launch fix #2574

Merged
merged 7 commits into from
Apr 5, 2022
Merged

Mpi launch fix #2574

merged 7 commits into from
Apr 5, 2022

Conversation

wxtim
Copy link
Contributor

@wxtim wxtim commented Mar 30, 2022

closes #2573

Summary of problem

Rose MPI launch fails if ROSE_LAUNCHER contains spaces.

wxtim added 2 commits March 30, 2022 16:00
- [x] Test added - `t/rose-mpi-launch/03-commands.t`
@wxtim wxtim self-assigned this Mar 30, 2022
@wxtim wxtim added the small label Mar 30, 2022
@wxtim wxtim added this to the 2.0rc3 milestone Mar 30, 2022
@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 30, 2022

Possibly broken by shellcheck changes?

@oliver-sanders
Copy link
Member

I'm not sure I understand this solution:

    for command in ${ROSE_LAUNCHER}; do
        if  command="$(type -P "$command")"

I'm not sure each item in $ROSE_LAUNCHER is supposed to be a separate command?

I think this was probably broken by shellcheck changes (my bad) because otherwise this script is more or less unchanged.

Here's the full diff to Rose 2019:

--- mpi.2019.01.x	2022-03-31 15:36:09.771016000 +0100
+++ mpi.master	        2022-03-31 15:36:40.450043000 +0100
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 #-------------------------------------------------------------------------------
-# Copyright (C) 2012-2020 British Crown (Met Office) & Contributors.
+# Copyright (C) British Crown (Met Office) & Contributors.
 #
 # This file is part of Rose, a framework for meteorological suites.
 #
@@ -54,7 +54,8 @@
 #
 # CONFIGURATION
 #     The command reads from the `[rose-mpi-launch]` section in
-#     `$ROSE_HOME/etc/rose.conf` and `$HOME/.metomi/rose.conf`.
+#     the Rose configuration.
+#
 #     Valid settings are:
 #
 #     launcher-list=LIST
@@ -111,14 +112,23 @@
 # DIAGNOSTICS
 #     Return 0 on success, 1 or exit code of the launcher program on failure.
 #-------------------------------------------------------------------------------
-. $(dirname $0)/../lib/bash/rose_init
-rose_init rose_log
+set -eu
+# shellcheck source=metomi/rose/etc/lib/bash/rose_log
+. "$(rose resource lib/bash/rose_log)"
+# shellcheck source=metomi/rose/etc/lib/bash/rose_usage
+. "$(rose resource lib/bash/rose_usage)"
+
+ROSE_CMD="${ROSE_NS}-${ROSE_UTIL}"
 
 # ------------------------------------------------------------------------------
 ROSE_COMMAND=
 ROSE_COMMAND_FILE=
 while (($# > 0)); do
     case $1 in
+    --help)
+        rose_help
+        exit
+        :;;
     --command-file=*)
         ROSE_COMMAND_FILE=${1#*=}
         shift 1
@@ -134,6 +144,9 @@
     --inner)
         shift 1
         if [[ -n ${ROSE_LAUNCHER_ULIMIT_OPTS:-} ]]; then
+            # shellcheck disable=SC2086
+            # permit word splitting in ROSE_LAUNCHER_ULIMIT_OPTS
+            # alternative would be to use an array
             while getopts 'HST:ab:c:d:e:f:i:l:m:n:p:q:r:s:t:u:v:x:' \
                 OPT $ROSE_LAUNCHER_ULIMIT_OPTS
             do
@@ -189,14 +202,14 @@
     if (($# < 1)); then
         rose_usage 1
     fi
-    if ! ROSE_COMMAND=$(type -P $1); then
+    if ! ROSE_COMMAND="$(type -P "$1")"; then
         err "$1: COMMAND not found"
     fi
     shift 1
 fi
 
 ROSE_LAUNCHER_LIST=${ROSE_LAUNCHER_LIST:-$( \
-    rose config --default= $ROSE_NS launcher-list)}
+    rose config --default= "$ROSE_CMD" launcher-list)}
 export NPROC=${NPROC:-1}
 
 #-------------------------------------------------------------------------------
@@ -211,7 +224,7 @@
 if ! printenv ROSE_LAUNCHER >/dev/null; then
     ROSE_LAUNCHER=
     for LAUNCHER in $ROSE_LAUNCHER_LIST; do
-        if type -P $LAUNCHER >/dev/null; then
+        if type -P "$LAUNCHER" >/dev/null; then
             ROSE_LAUNCHER=$LAUNCHER
             break
         fi
@@ -224,11 +237,11 @@
 ROSE_LAUNCHER_BASE=
 if [[ -n $ROSE_LAUNCHER ]]; then
     # Path
-    if ! ROSE_LAUNCHER_PATH=$(type -P $ROSE_LAUNCHER); then
+    if ! ROSE_LAUNCHER_PATH="$(type -P "$ROSE_LAUNCHER")"; then
         err "ROSE_LAUNCHER: $ROSE_LAUNCHER: command not found"
     fi
     ROSE_LAUNCHER=$ROSE_LAUNCHER_PATH
-    ROSE_LAUNCHER_BASE=$(basename $ROSE_LAUNCHER)
+    ROSE_LAUNCHER_BASE="$(basename "$ROSE_LAUNCHER")"
 fi
 
 #-------------------------------------------------------------------------------
@@ -239,9 +252,10 @@
         err "ROSE_LAUNCHER not defined, command file not supported."
     fi
     ROSE_LAUNCHER_FILEOPTS=${ROSE_LAUNCHER_FILEOPTS:-$( \
-        rose config --default= $ROSE_NS launcher-fileopts.$ROSE_LAUNCHER_BASE)}
-    eval "info 2 exec $ROSE_LAUNCHER $ROSE_LAUNCHER_FILEOPTS $@"
-    eval "exec $ROSE_LAUNCHER $ROSE_LAUNCHER_FILEOPTS $@"
+        rose config --default= \
+        "$ROSE_CMD" "launcher-fileopts.$ROSE_LAUNCHER_BASE")}
+    eval "info 2 exec $ROSE_LAUNCHER $ROSE_LAUNCHER_FILEOPTS $*"
+    eval "exec $ROSE_LAUNCHER $ROSE_LAUNCHER_FILEOPTS $*"
 else
     if [[ -n $ROSE_LAUNCHER_BASE ]]; then
         ROSE_LAUNCH_INNER=
@@ -250,9 +264,9 @@
         fi
         # Options
         ROSE_LAUNCHER_PREOPTS=${ROSE_LAUNCHER_PREOPTS:-$(rose config -E \
-            --default= $ROSE_NS launcher-preopts.$ROSE_LAUNCHER_BASE)}
+            --default= "$ROSE_CMD" "launcher-preopts.$ROSE_LAUNCHER_BASE")}
         ROSE_LAUNCHER_POSTOPTS=${ROSE_LAUNCHER_POSTOPTS:-$(rose config -E \
-            --default= $ROSE_NS launcher-postopts.$ROSE_LAUNCHER_BASE)}
+            --default= "$ROSE_CMD" "launcher-postopts.$ROSE_LAUNCHER_BASE")}
     else
         ROSE_LAUNCH_INNER=
         ROSE_LAUNCHER_PREOPTS=
@@ -261,8 +275,10 @@
     if ((ROSE_VERBOSITY >= 3)); then
         info 3 printenv
         printenv | sort
-        run ldd $ROSE_COMMAND || true 2>/dev/null
+        run ldd "$ROSE_COMMAND" || true 2>/dev/null
     fi
+    # shellcheck disable=SC2086
+    # permit word splitting in command / argument strings
     info 2 exec \
         $ROSE_LAUNCHER \
         $ROSE_LAUNCHER_PREOPTS \
@@ -270,6 +286,8 @@
         "$ROSE_COMMAND" \
         $ROSE_LAUNCHER_POSTOPTS \
         "$@"
+    # shellcheck disable=SC2086
+    # permit word splitting in command / argument strings
     exec \
         $ROSE_LAUNCHER \
         $ROSE_LAUNCHER_PREOPTS \

My guess would be the quotes added around LAUNCHER & ROSE_LAUNCHER variables should not have been added.

@wxtim
Copy link
Contributor Author

wxtim commented Mar 31, 2022

I'm not sure each item in $ROSE_LAUNCHER is supposed to be a separate command?

This is the case with the examples in the issue : @dpmatthews - are the items in $ROSE_LAUNCHER separate commands?

@wxtim
Copy link
Contributor Author

wxtim commented Apr 1, 2022

My guess would be the quotes added around LAUNCHER & ROSE_LAUNCHER variables should not have been added.

I think you are right about ROSE_LAUNCHER, but after talking with @dpmatthews I'm not removing quotes from LAUNCHER because I don't think that we want to split launcher - It is set in a for loop which appears to rely on word splitting from ROSE_LAUNCHER_LIST.

Copy link
Member

@dpmatthews dpmatthews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the fix but I haven't checked the new test

@oliver-sanders
Copy link
Member

Do we have a real-world example we can try this out against?

@dpmatthews
Copy link
Member

The example problems I gave in #2573 were both taken from real world examples. This change fixes those problems.

@oliver-sanders
Copy link
Member

Ok going in.

@oliver-sanders oliver-sanders merged commit 5d033aa into metomi:master Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rose mpi-launch breaks if ROSE_LAUNCHER contains spaces
3 participants